feat(edge): detect + speak WHOOP 5 (gen5) alongside WHOOP 4 - #97
feat(edge): detect + speak WHOOP 5 (gen5) alongside WHOOP 4#97abdulsaheel wants to merge 12 commits into
Conversation
Scan filters both service UUIDs (gen4 6108xxxx / gen5 fd4bxxxx); at discovery the session pins its generation and rebuilds the frame reassemblers with the matching header shape. The BandProfile is threaded through the frame path, the command builder, and the history-result ACK (the safe-trim token echo). Adds the gen5 handshake branch (client-hello + empty-payload offload) and routes gen5 records through parseGen5Record; unknown/motion kinds fall through to raw_archive as before. The WHOOP 4 path is unchanged. The gen5 connect/handshake path is not yet validated on physical hardware (marked in-code) — pending a WHOOP 5 band.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughBLE scanning, service discovery, protocol handling, initialization, historical decoding, diagnostics, and iOS accessory selection now support WHOOP gen4 and gen5 bands through generation-specific profiles, commands, UUIDs, and transfer flows. ChangesWHOOP generation support
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant BLEScanner
participant ServiceDiscovery
participant BLESession
participant WHOOPBand
participant HistoricalDecoder
BLEScanner->>ServiceDiscovery: discover gen4 or gen5 WHOOP services
ServiceDiscovery->>BLESession: applyBand(BandProfile)
BLESession->>BLESession: rebuild frame reassemblers
BLESession->>WHOOPBand: send generation-specific initialization
BLESession->>WHOOPBand: request generation-specific historical data
WHOOPBand-->>BLESession: return framed historical records
BLESession->>HistoricalDecoder: decode records using pinned profile
HistoricalDecoder-->>BLESession: return supported Sample records
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/ble/ble_engine.dart`:
- Around line 2380-2394: Centralize the generation-specific GET_DATA_RANGE and
SEND_HISTORICAL_DATA writes in a helper, using empty gen5 payloads and the
existing gen4 payloads as appropriate. Update the gen5 initial flow near
gen5ClientHello() and _startHistoricalRefresh() to call this helper so periodic,
manual, and retry refreshes preserve the gen5 command format.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: fd244272-7c1d-4b50-b23c-9dbae531602f
📒 Files selected for processing (1)
lib/ble/ble_engine.dart
…paths CodeRabbit review: the initial handshake used gen5 empty-payload GET_DATA_RANGE/SEND_HISTORICAL_DATA, but the periodic backfill, manual refresh, and retry paths still sent the gen4 [0x00] payload (only the frame envelope was band-correct). Extract _sendGetDataRange / _sendHistoricalData helpers that pick the payload by generation (gen4 [0x00], gen5 empty) and route the init, refresh, backfill, and retry paths through them, so the gen5 offload format is identical everywhere.
|
Addressed in 67fa1cb — extracted @coderabbitai review |
|
✅ Action performedReview finished.
|
…t gen5 branch The BLE engine could speak gen5, but a WHOOP 5 band could not be paired on iOS: AccessorySetupKit only advertised the gen4 6108 service, so a fd4b band never appeared in the picker (and with no ASK provisioning the restore central is never created → no connection at all). - AccessorySetup.swift: offer one ASPickerDisplayItem per generation (gen4 6108 + gen5 fd4b) so either band can be provisioned; the provisioned CoreBluetooth identifier is generation-agnostic. - Info.plist: add the gen5 service to NSAccessorySetupBluetoothServices (required for the descriptor criterion). - pubspec.yaml: point openstrap_protocol at feat/multiband-whoop5 for the experimental build (revert to main once protocol#16 merges). Android needs no change: CDM associates by MAC (generation-agnostic) and the Flutter scan is already fd4b-aware. iOS restore reconnects by peripheral identifier, also generation-agnostic. Still hardware-unvalidated end-to-end — pending a physical WHOOP 5 band.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pubspec.yaml (1)
27-29: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winBranch ref makes builds non-reproducible; pin a commit for release.
Using
ref: feat/multiband-whoop5means a force-push to that branch silently changes the resolved dependency, making builds non-reproducible. This is acceptable for experimental development, but before any release or merge to a production branch, pin to a specific commit SHA instead of a branch ref.🔒️ Suggested pinning for reproducibility
openstrap_protocol: git: url: https://github.com/OpenStrap/protocol.git - # EXPERIMENTAL: WHOOP 5 (gen5) multi-band support. Point back to `main` - # once OpenStrap/protocol#16 merges. - ref: feat/multiband-whoop5 + # EXPERIMENTAL: WHOOP 5 (gen5) multi-band support. Point back to `main` + # once OpenStrap/protocol#16 merges. + # Pinned to a specific commit for reproducibility; update intentionally. + ref: <commit-sha-of-branch-tip>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pubspec.yaml` around lines 27 - 29, Replace the branch-based ref for the OpenStrap dependency in the WHOOP 5 experimental configuration with the specific reviewed commit SHA before release or production-branch merge. Keep the dependency source and existing experimental intent unchanged, and remove the non-reproducible feat/multiband-whoop5 branch reference.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@pubspec.yaml`:
- Around line 27-29: Replace the branch-based ref for the OpenStrap dependency
in the WHOOP 5 experimental configuration with the specific reviewed commit SHA
before release or production-branch merge. Keep the dependency source and
existing experimental intent unchanged, and remove the non-reproducible
feat/multiband-whoop5 branch reference.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 0bd7319d-ad6c-41b3-85ec-f7333f0d0e93
📒 Files selected for processing (3)
ios/Runner/AccessorySetup.swiftios/Runner/Info.plistpubspec.yaml
Pin the committed lock's openstrap_protocol dependency to the gen5 branch commit (687aa46) so CI/release resolves the exact experimental protocol revision. Only the protocol ref/resolved-ref changed; analytics stays on main. (Locally the gitignored pubspec_overrides.yaml still redirects to ../protocol for dev; the committed lock is what release resolution uses.)
|
I have a whoop 5 that I am willing to use to validate against if that would help? |
# Conflicts: # lib/ble/ble_engine.dart # pubspec.lock # pubspec.yaml
PR Reviewer Guide 🔍(Review updated until commit 82b094c)Here are some key observations to aid the review process:
|
PR Code Suggestions ✨ |
…ted engine Extends BleEngine (not a parallel copy) so a gen5 link gets a real, working lifecycle end to end instead of stopping at "connected": - SET_CLOCK/GET_CLOCK now use gen5's own opcodes (SET_CLOCK_MAVERICK/ GET_CLOCK_GEN5) instead of silently sending gen4's — the bug that would have left a gen5 strap's RTC forever unlatched and refusing history. - Historical-record ingestion now calls protocol's real parseGen5Historical (v18/v20/v21/v26) via a new sampleFromGen5Historical mapper, replacing the old parseGen5Record call that targeted gen4's version numbers and would have silently archived every real gen5 record. - Added the opt-in R22 deep-buffer enable sequence (default OFF, gated by a constructor toggle) and a gen5 Maverick haptic buzz path. - _send now also blocks OpcodeSafety.destructive band-agnostically, alongside the existing gen4 dangerousCmds list. - decodeFrame is now called with the session's BandProfile so gen5's direct-percent battery / GET_HELLO shape decode correctly; added a small edge-side augment for GET_CLOCK_GEN5's clock_epoch (protocol doesn't populate it yet) and debug-only logging for gen5 console/hello frames. - DeviceState.generation + a band_generation ledger field surface which WHOOP generation a session/batch came from, with no schema migration (rides the existing sync_ledger meta_json blob). - New gen5_sample_mapping_test.dart covers the v18->Sample mapping against a real byte-verified capture, plus the deep-buffer/null fall-through. gen4 behavior is unchanged (every branch above is band-gated); full suite green (1059 tests, 2 pre-existing skips). Real-hardware validation of the handshake and R22 sequence is still outstanding — see inline doc comments.
The previous pin (7f1a2db) predates protocol's real v18/v20/v21/v26 decoders, gen5 clock/haptics/SET_CONFIG opcodes, R22 sequence builder, OpcodeSafety gate, and CONSOLE_LOGS decoder — all of which the previous edge commit's BleEngine changes call directly. Verified with a clean `flutter pub get` (no local path override) against this SHA: analyze and the full test suite (1059 tests) both green.
|
Persistent review updated to latest commit 9d13f1b |
|
Persistent review updated to latest commit aa18782 |
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
lib/ble/ble_engine.dart (2)
970-992: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winUse
GattProfileprefixes instead of hardcoded magic strings for scan matching.The scan callback hardcodes
'61080001'and'fd4b0001'as service-UUID prefixes. Service discovery at Line 1173 and Line 1178 usesGattProfile.gen4.servicePrefixandGattProfile.gen5.servicePrefixfor the same purpose. Two sources of truth exist for identical values. If either prefix constant changes, the scan's early-stop optimization silently desyncs from the actual discovery match, and the scan runs to its full timeout instead of stopping early.Reference the same constants in both places.
♻️ Proposed fix
if (found == null && (name.contains('whoop') || advNames.any((s) => - s.startsWith('61080001') || s.startsWith('fd4b0001')))) { + s.startsWith(GattProfile.gen4.servicePrefix.toLowerCase()) || + s.startsWith(GattProfile.gen5.servicePrefix.toLowerCase())))) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/ble/ble_engine.dart` around lines 970 - 992, Update the scan callback in the BLE discovery flow to replace the hardcoded “61080001” and “fd4b0001” checks with GattProfile.gen4.servicePrefix and GattProfile.gen5.servicePrefix, matching the constants used by service discovery. Keep the existing case-normalized prefix matching and early-stop behavior unchanged.
1426-1426: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAdd regression coverage for the gen5 refresh offload payload.
lib/ble/ble_engine.dartsends[0x00]for gen4 and[]for gen5, but the test suite only exercises gen5 historical mapping throughtest/gen5_sample_mapping_test.dart. Add a gen5-pinned refresh/backfill/retry regression that assertsGET_DATA_RANGE/SEND_HISTORICAL_DATAare sent with an empty payload, not only the initialsendInitpath.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/ble/ble_engine.dart` at line 1426, Add regression coverage in the gen5 refresh/backfill/retry test flow around _sendGetDataRange, pinning the device generation to gen5 and asserting GET_DATA_RANGE and SEND_HISTORICAL_DATA use empty payloads. Exercise the refresh path beyond the initial sendInit sequence, including backfill and retry behavior, while preserving the existing gen4 payload expectations.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pubspec.yaml`:
- Around line 38-44: Update the stale protocol-generation comments near the
WHOOP dependency to describe the active experimental WHOOP 5 multi-band
configuration and pinned protocol commit, removing any WHOOP-4-only or ref: main
claims. Keep the dependency pinned to the existing full commit SHA and do not
introduce a branch reference.
- Around line 38-44: Before release, validate the pinned experimental protocol
from the WHOOP 5 flow through gen5 connection, handshake, and R22 behavior on
physical hardware, including GATT discovery and device-specific framing; update
the release readiness or dependency decision around the pubspec ref only after
those checks pass.
- Around line 38-44: The protocol dependency pin in the pubspec must reference a
commit that actually contains the claimed gen5 framing and CRC implementation
changes. Update the ref under the WHOOP 5 multi-band dependency comments to the
correct SHA containing the crc8 length-field validation, odd-length hex
rejection, and profile-aware header-CRC fixes; preserve the pinned-commit
approach.
---
Outside diff comments:
In `@lib/ble/ble_engine.dart`:
- Around line 970-992: Update the scan callback in the BLE discovery flow to
replace the hardcoded “61080001” and “fd4b0001” checks with
GattProfile.gen4.servicePrefix and GattProfile.gen5.servicePrefix, matching the
constants used by service discovery. Keep the existing case-normalized prefix
matching and early-stop behavior unchanged.
- Line 1426: Add regression coverage in the gen5 refresh/backfill/retry test
flow around _sendGetDataRange, pinning the device generation to gen5 and
asserting GET_DATA_RANGE and SEND_HISTORICAL_DATA use empty payloads. Exercise
the refresh path beyond the initial sendInit sequence, including backfill and
retry behavior, while preserving the existing gen4 payload expectations.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 118d5963-8206-46f1-8518-4e8f4e54f1af
⛔ Files ignored due to path filters (1)
pubspec.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
ios/Runner/Info.plistlib/ble/ble_engine.dartlib/data/models.dartpubspec.yamltest/gen5_sample_mapping_test.dart
PR Code Suggestions ✨ |
1 similar comment
PR Code Suggestions ✨ |
|
Persistent review updated to latest commit e21bbaf |
PR Code Suggestions ✨ |
|
Persistent review updated to latest commit 260cd0c |
PR Code Suggestions ✨ |
|
Persistent review updated to latest commit 63a87ed |
|
Persistent review updated to latest commit 82b094c |
PR Code Suggestions ✨ |
PR Code Suggestions ✨ |
|
I was able to connect with my whoop 5. so far i can see heart rate data live. I will keep it connected to see recovery/strain and other data points. |
Great !! Thanks, lets continue this discussion here: #178 |
3 participants
Summary
Teaches the BLE engine to detect and talk to WHOOP 5 (gen5 / "fd4b") bands as well as WHOOP 4. Pairs with OpenStrap/protocol#16 (the
BandProfileabstraction). Analytics needs no changes — WHOOP 5 just feeds the samedecoded_onehz/decoded_rrtables.Changes (all in
lib/ble/ble_engine.dart)6108xxxx/ gen5fd4bxxxx)._Session.applyBand) and rebuilds the frame reassemblers with the matching header shape; characteristics resolved via the generation's GATT prefixes.BandProfilethreaded through the frame path, the command builder (_send), and the history-result ACK (safe-trim token echo).sendInit: client-hello (0x91) + empty-payloadGET_DATA_RANGE/SEND_HISTORICAL_DATA.parseGen5Record; unknown/motion kinds fall through toraw_archiveas before.Testing
flutter test --concurrency=1→ 483 pass (no regression).flutter analyze lib/ble/→ clean.Invariants preserved
decoded_*schema unchanged.The gen5 connect/handshake path is not yet validated on physical hardware (no WHOOP 5 band available) — clearly marked in-code. Decode is validated against real owned captures + synthetic vectors. Requires an on-device pairing pass (bond → handshake → offload → ACK → trim → decode → day derive) before shipping to gen5 users.
🤖 Generated with Claude Code
Summary by CodeRabbit